Skip to content

simplify dispatcher if-elif #7084

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 16, 2023
Merged

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Jan 13, 2023

@pmeier pmeier requested review from NicolasHug and vfdev-5 January 13, 2023 09:05
@pmeier
Copy link
Collaborator Author

pmeier commented Jan 13, 2023

This works at runtime and with JIT, but mypy is not happy. Since we completely ignore PIL

vision/mypy.ini

Lines 120 to 122 in 035d99f

[mypy-PIL.*]
ignore_missing_imports = True

it treats all annotations with PIL.Image.Image as Any. Meaning, although we correctly annotate

def gaussian_blur_image_pil(
image: PIL.Image.Image, kernel_size: List[int], sigma: Optional[List[float]] = None
) -> PIL.Image.Image:

for mypy this looks like def gaussian_blur_image_pil(...) -> Any. Subsequently, it complains that we return Any from a function annotated with torch.Tensor:

elif isinstance(inpt, PIL.Image.Image):
return gaussian_blur_image_pil(inpt, kernel_size=kernel_size, sigma=sigma)

I'm not sure how this worked before, since in any case due to our JIT annotation, neither PIL.Image.Image nor Any should have been allowed. Maybe the convoluted check was designed in a way to "confuse" mypy to not complain?

I see three ways forward here:

  1. Keep everything as is, since it works. Given the situation that might be brittle, since mypy maybe at some point can look through the convoluted check and will complain then.
  2. Add a # type: ignore on all (27) of the PIL returns in the dispatchers.
  3. Drop the strict
    warn_return_any = True

    configuration.

My preference is 3. -> 2. -> 1.

@NicolasHug
Copy link
Member

Thanks for looking into it Philip. I agree with the order of preference and I'm happy with 3.

@pmeier pmeier marked this pull request as ready for review January 16, 2023 07:35
@@ -46,7 +46,7 @@ class ToImageTensor(Transform):
def _transform(
self, inpt: Union[torch.Tensor, PIL.Image.Image, np.ndarray], params: Dict[str, Any]
) -> datapoints.Image:
return F.to_image_tensor(inpt) # type: ignore[no-any-return]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to the relaxed strictness, this one (and the ones below) became obsolete.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot Philip, LGTM.

I have 2 follow-up questions / comments, which we can address (or not) in other PRs:

  • As discussed offline, JIT "isn't supported" for the datapoints on these dispatchers. What we mean by "not supported" is that the datapoints will be treated as a pure Tensors, and the output will be a Tensor too. I assume this can lead to unexpected results and confusion, e.g.:

    mask = Mask(...)
    out = T(mask)
    # - unaware user expects a Mask but gets a Tensor
    # - `T` treats mask as an image, not as a mask!
    #    Might result in different behaviour (very bad) or confusing error message?

    so: should we fail loudly with a good error message in the disaptchers when is_scripting() is True and is_simple_tensor() is False?

  • Side note: should we be more lenient with mypy checks in the prototype area?
    We introduced strict checks in make mypy more strict for prototype datasets #4513 (and I approved the PR, so I'll concede I'm sort of rebutting myself here).
    But the the prototype area is where we want to be able to move fast, and try potentially crazy things without having to worry too much about type checks / docs / super clean tests. In this specific case, the introduction of warn_return_any could have been the reason for these convoluted checks in the first place which complicates the code and slows down iteration speed. Perhaps we may want to relax the mypy requirements in order to experiment faster in the prototype area?

@pmeier
Copy link
Collaborator Author

pmeier commented Jan 16, 2023

  1. I'm looking into it. is_simple_tensor seems to not behave as we want it to be when scripting. Will keep you posted.
  2. Yes, I'm ok with that.

@pmeier
Copy link
Collaborator Author

pmeier commented Jan 16, 2023

@pmeier pmeier merged commit f71c430 into pytorch:main Jan 16, 2023
@pmeier pmeier deleted the improve-dispatcher-if branch January 16, 2023 12:43
facebook-github-bot pushed a commit that referenced this pull request Jan 24, 2023
Reviewed By: YosuaMichael

Differential Revision: D42706906

fbshipit-source-id: 7f9db8942b704e35054fb842e806dd016db127a2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants